Migrate from backoff to backon
#1652
Conversation
Fixes kube-rs#1635 Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
|
|
Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
|
/cc @Xuanwo |
This seems to have been added in Rust 1.83 that was just released, and is only triggered by tarpaulin? Signed-off-by: Natalie Klestrup Röijezon <[email protected]>
|
Hm, really not sure about what's still making tarpaulin unhappy.. :/ |
| //! Delays and deduplicates [`Stream`](futures::stream::Stream) items | ||
| //! Delays and deduplicates [`Stream`] items |
There was a problem hiding this comment.
has rustdoc gotten smarter lately? this stuff used to give broken links when running just doc
There was a problem hiding this comment.
Ah, damn. You're right.. clippy lead me astray. Will revert.
There was a problem hiding this comment.
Nevermind, this actually seems to have come from the --no-deps flag given by just doc. Removing that generates the links correctly.
There was a problem hiding this comment.
hopefully this is how docs.rs builds docs. i tried to find some info on this, but not much on their docs. it is possible to inject rustdoc args though, so should probably be fine
There was a problem hiding this comment.
Looks like the code that controls the arguments is https://github.com/rust-lang/docs.rs/blob/5ec4326126b2f244442f55f0183ca7523611f0df/crates/metadata/lib.rs#L253-L300. Building it as RUSTC_BOOTSTRAP=1 cargo rustdoc --lib -Zrustdoc-map --all-features -- --cfg docsrs does seem to work, and links to the corresponding docs.rs entries.
| /// Builder for [`ResetTimerBackoff`]. | ||
| #[derive(Debug, Clone)] | ||
| pub struct ResetTimerBackoffBuilder<B> { |
There was a problem hiding this comment.
I do see the other PR is sidestepping this by adding a trait instead. https://github.com/kube-rs/kube/pull/1653/files#diff-32fcc57da3d0b322998a85b05a1a82dae3f76df733b8a878efa1c97695af386eL3-R13
not sure if this is better or not, but it's possibly less abstraction overhead?
There was a problem hiding this comment.
That PR requires you to implement its Backoff trait manually for each use case, which adds a lot of (code) overhead, especially for downstream users that would be subject to the orphaning rules.
There was a problem hiding this comment.
True, but are there realistically that many people that would need to tweak backoff behavior? Do we need to optimise for those people? We don't have examples/ to show people how to do it (which to me is indicative that this is not super high value). I can only think of people trying to use it as a half-baked solution to extremely tight consistency guarantees (by hammering the api-server more during network stress) - and that feels not like something we want to encourage. Maybe I am missing something?
The downside to this is complexity of our side; 4 types that we expose from utils/mod (with relations that are hard to decipher), a generic type that sneaks its way into type signatures (in an already complicated controller/mod).
There was a problem hiding this comment.
True, but are there realistically that many people that would need to tweak backoff behavior?
It just feels weird that it becomes.. almost composable, but not quite. Especially since there ends up being a lot of stutter in the implementation (looking at their PR, they have to hard-code the same constants in both ::new() and ::reset()).
a generic type that sneaks its way into type signatures (in an already complicated controller/mod).
We should be able to hide that by integrating it into the implementation of StreamBackoff.
There was a problem hiding this comment.
We should be able to hide that by integrating it into the implementation of StreamBackoff.
Ah right no, since BackoffBuilder doesn't make sense as a trait object, so that complicates the type of Controller.
|
not sure about tarpaulin either. ci is has tarpaulin pinned to 0.28.0 (so should not be a new version thing), and it should otherwise run equivalently locally with |
|
Might be related to this error in the middle of the compile log.. That doesn't happen if I use the Rust 1.82.0 toolchain instead (and tarpaulin completes successfully). |
|
#1658 seems to have sorted out the CI issues at least. |
Motivation
backoff has been unmaintained for a while now, which is starting to cause rustsec warnings (#1635). backon is a maintained alternative.
Fixes #1635.
Solution
This PR migrates all uses of backoff to backon. This is a pretty breaking change for anyone who interacts with our backoff system beyond letting
Controlleruse its default.backon::Backoffdoesn't have an equivalent tobackoff::Backoff::reset, so this PR adds a newResettableBackoffwrapper system for those use cases.